Skip to content

Conversation

@bsermons
Copy link

@bsermons bsermons commented Jul 12, 2016

Wasn't sure if there was a more appropriate way to append two NonEmpty's so this seemed useful.

@paf31
Copy link
Contributor

paf31 commented Jul 13, 2016

I think this type should have a Semigroup instance, but I'm not sure if this is the right one. There are at least two more:

(Semigroup a, Applicative f) => Semigroup (NonEmpty f a)
Alternative f => Semigroup (NonEmpty f a)

@bsermons
Copy link
Author

Ok, I'm not sure the difference between those instances really. I just needed Applicative's pure to get it to work and played around until it compiled. I'll be happy to update if you tell me which is more appropriate.

@sharkdp
Copy link
Contributor

sharkdp commented Jul 13, 2016

Just to compare, here are all three versions (assuming this is what @paf31 meant):

instance i1 :: (Applicative f, Semigroup (f a)) => Semigroup (NonEmpty f a) where
   append (a :| fa) (b :| fb) = a :| (fa <> pure b <> fb)

instance i2 :: Alternative f => Semigroup (NonEmpty f a) where
  append (a :| fa) (b :| fb) = a :| (fa <|> pure b <|> fb)

instance i3 :: (Semigroup a, Applicative f) => Semigroup (NonEmpty f a) where
   append (a :| fa) (b :| fb) = (a <> b) :| (append <$> fa <*> fb)

The first two are the same for f = Array:

("1" :| ["2", "3"]) <> ("4" :| ["5"]) == "1" :| ["2", "3", "4", "5"]

while the latter fulfills:

("1" :| ["2", "3"]) <> ("4" :| ["5"]) == "14" :| ["25", "35"]

@wereHamster
Copy link

wereHamster commented Nov 9, 2016

I think the first option (Applicative f, Semigroup (f a)) is more appropriate. It mimics the behaviour of Array and other container types where (<>) concatenates the two containers rather than the individual items within.

Also, in the last example, when the two containers don't have the same length, the behaviour is rather weird. Especially with regard to the last element in the resulting list. You're probably better off doing something based on map+zip.

@paf31
Copy link
Contributor

paf31 commented Dec 24, 2016

Closing this until we can decide which instance is the right one.

@paf31 paf31 closed this Dec 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants